feat(auto-zoom): configurable zoom with edge snapping, input filtering, and adaptive intensity#1667
Open
namearth5005 wants to merge 11 commits intoCapSoftware:mainfrom
Open
feat(auto-zoom): configurable zoom with edge snapping, input filtering, and adaptive intensity#1667namearth5005 wants to merge 11 commits intoCapSoftware:mainfrom
namearth5005 wants to merge 11 commits intoCapSoftware:mainfrom
Conversation
…nfigurable fields
…placing hardcoded constants
- Remove #[allow(dead_code)] from apply_edge_snap_to_focus - Apply edge snapping per-frame in render pipeline for Auto mode segments - Add edge_snap_enabled config field (default true) - Set edge_snap_ratio to 0.0 on generated segments when disabled - Add Edge Snapping toggle in experimental settings UI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+176
to
+193
| <SettingSlider | ||
| label="Min Zoom" | ||
| value={settings.autoZoomConfig?.minZoomAmount ?? 1.2} | ||
| onChange={(v) => handleConfigChange("minZoomAmount", v)} | ||
| min={1.0} | ||
| max={3.0} | ||
| step={0.1} | ||
| format={(v) => `${v.toFixed(1)}x`} | ||
| /> | ||
| <SettingSlider | ||
| label="Max Zoom" | ||
| value={settings.autoZoomConfig?.maxZoomAmount ?? 2.5} | ||
| onChange={(v) => handleConfigChange("maxZoomAmount", v)} | ||
| min={1.5} | ||
| max={4.0} | ||
| step={0.1} | ||
| format={(v) => `${v.toFixed(1)}x`} | ||
| /> |
Contributor
There was a problem hiding this comment.
The two sliders are fully independent: Min Zoom goes up to 3.0× and Max Zoom starts at 1.5×, so a user can set Min Zoom = 3.0 and Max Zoom = 1.5.
When min_zoom_amount > max_zoom_amount, compute_zoom_amount produces inverted results:
- A tight cluster of clicks (small
max_dist) returnsmax_zoom_amount(1.5×) — i.e. minimal zoom. - Spread activity (large
max_dist) returnsmin_zoom_amount(3.0×) — i.e. maximum zoom.
The effect is completely backwards from the labelling. Consider clamping or cross-validating on change:
onChange={(v) => {
const clamped = Math.min(v, settings.autoZoomConfig?.maxZoomAmount ?? 4.0);
handleConfigChange("minZoomAmount", clamped);
}}and symmetrically for the Max Zoom slider:
onChange={(v) => {
const clamped = Math.max(v, settings.autoZoomConfig?.minZoomAmount ?? 1.0);
handleConfigChange("maxZoomAmount", clamped);
}}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx
Line: 176-193
Comment:
**No validation that Max Zoom ≥ Min Zoom**
The two sliders are fully independent: Min Zoom goes up to 3.0× and Max Zoom starts at 1.5×, so a user can set Min Zoom = 3.0 and Max Zoom = 1.5.
When `min_zoom_amount > max_zoom_amount`, `compute_zoom_amount` produces *inverted* results:
- A tight cluster of clicks (small `max_dist`) returns `max_zoom_amount` (1.5×) — i.e. minimal zoom.
- Spread activity (large `max_dist`) returns `min_zoom_amount` (3.0×) — i.e. maximum zoom.
The effect is completely backwards from the labelling. Consider clamping or cross-validating on change:
```tsx
onChange={(v) => {
const clamped = Math.min(v, settings.autoZoomConfig?.maxZoomAmount ?? 4.0);
handleConfigChange("minZoomAmount", clamped);
}}
```
and symmetrically for the Max Zoom slider:
```tsx
onChange={(v) => {
const clamped = Math.max(v, settings.autoZoomConfig?.minZoomAmount ?? 1.0);
handleConfigChange("maxZoomAmount", clamped);
}}
```
How can I resolve this? If you propose a fix, please make it concise.
namearth5005
changed the title
…-zero guards - Add AutoZoomConfig::validated() to clamp min/max zoom and guard intensity_spatial_scale - Replace silent unwrap_or chains with proper error logging via tracing::error - Guard division by zero in apply_edge_snap_to_focus for zoom_amount < epsilon - Guard 0/0 NaN in compute_zoom_amount with intensity_spatial_scale.max(EPSILON) - Deduplicate edge-snap rendering block into closure - Remove dead auto_zoom_amount variable - Add await/catch to handleConfigChange for store persistence - Fix fragile right_click_ignored test (movement events were masking assertion)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AutoZoomConfigstruct with user-facing settings (min/max zoom, sensitivity, smoothing, edge snapping)Changes
crates/project/src/configuration.rsAutoZoomConfigstruct with 18 configurable fields and serde defaultsapps/desktop/src-tauri/src/general_settings.rsAutoZoomConfigintoGeneralSettingsStoreapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src/routes/.../experimental.tsxcrates/rendering/src/lib.rsapply_edge_snap_to_focusfor auto-zoom segmentscrates/rendering/src/zoom_focus_interpolation.rs#[allow(dead_code)]since function is now usedTest plan
AutoZoomConfigmatches prior behaviorcargo testfor new unit tests (dead zone, dedup, right-click, intensity scaling)